-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement stats calculations for new calculated_stats_
container
#468
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nearly there, I think. I do think it would be worth adding a test for the calculate_index
function though and I've made a few other small suggestions.
The Windows CI runner is failing because of some type narrowing warning again 😞
src/HealthGPS/analysis_module.h
Outdated
std::vector<std::string> channels_; | ||
std::unordered_map<std::string, int> channel_index_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might not be completely understanding what these data structures do... But am I right in thinking channel_index_
contains indexes into channels_
? If so, could you just make channels_
a std::map<std::string, std::string>
(I'm guessing it needs to be ordered...) and drop channels_index_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I'd probs have a go at #470 before merging this, just so we can be confident that the indexing works, but it's up to you. I don't think it needs lots of test cases -- just a few basic ones.
This PR adds much of the functionality to calculate the stats specified in
channels_
. A lot of the logic for this is copied from the existing code, but modified to use thecalculated_stats_
vector in place of the existingseries
object. As a reminder, this is necessary because the calculated stats are being stored in a flat vector (calculated_stats_
) which represents a multidimensional array of factors that the user wants to group by, as opposed to theseries
object. A result of this is the introduction of thecalculate_index
method, and theget_channel_index
method - both necessary to find the correct index of the factor bin, and "channel" stat in thecalculated_stats_
vector, respectively.